Fix/ Update search results when graph nodes change#249
Conversation
Preview Deployed!
Last updated at 2026-04-26T11:05:30Z |
|
The search is working, but the UI is flickering a lot |
|
hey @AgniveshChaubey ! thanks for the review and for catching that. I noticed the flickering as well. I've just pushed a new commit that resolves the UI flickering. |
📝 WalkthroughWalkthroughA new Changeset was added to document a patch release fixing a bug where search results failed to update when the schema changed while the search UI remained active. The GraphView component was modified to add Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
hey @AgniveshChaubey ! thanks for pointing that out. it was loading fine on my local because of the development environment catching up faster, but i see why it happened in the preview. pushed a fix to properly fit the view. instead of relying purely on a hardcoded timeout (of 100ms) inside the ResizeObserver, i added ReactFlow's native fitView prop. this forces it to inherently wait until all nodes are fully generated and measured in the DOM before trying to zoom out perfectly. this gets rid of the race condition causing it to appear zoomed in initially. let me know if it all looks good to you now! |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/GraphView.tsx (1)
460-464:⚠️ Potential issue | 🟡 MinorPotential double
fitViewwhen clearing search.The clear button immediately calls
fitViewand deselects nodes. WhensetSearchString("")triggers the debounced effect (after 300ms), the effect will also attempt to deselect nodes and callfitViewif any were selected.The effect's guard (
hasSelected) might prevent the secondfitViewif the button handler already deselected all nodes, but this depends on timing. Consider either:
- Removing the immediate
fitViewcall from the button handler and letting the effect handle it, or- Adding a flag/ref to skip the effect's fitView when the button was used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/GraphView.tsx` around lines 460 - 464, The clear button currently calls fitView immediately and also updates state (setSearchString, setNodes) which can trigger the debounced effect that may call fitView again; add a skip flag to avoid duplicate fits by creating a ref (e.g., skipFitOnClearRef) set to true inside the onClick handler before clearing state, and in the debounced effect that checks hasSelected/readies fitView, early-return if skipFitOnClearRef.current is true and then reset it to false; reference setSearchString, setNodes, fitView, hasSelected and the debounced effect when applying this change.
🧹 Nitpick comments (3)
src/components/GraphView.tsx (3)
66-94: Consider adding missing dependencies tonavigateMatch.The
useCallbackdependencies include only[matchedNodes], but the callback also usessetCenter,getZoom,setNodes, and constantsNODE_WIDTH,NODE_HEIGHT. While the setters and constants are stable,setCenterandgetZoomfromuseReactFlowshould technically be included.- [matchedNodes] + [matchedNodes, setCenter, getZoom, setNodes]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/GraphView.tsx` around lines 66 - 94, The navigateMatch useCallback currently lists only matchedNodes in its dependency array but also reads setCenter, getZoom, setNodes and NODE_WIDTH/NODE_HEIGHT; update the dependency array for the navigateMatch callback to include setCenter, getZoom, setNodes, NODE_WIDTH, and NODE_HEIGHT (in addition to matchedNodes) so the hook correctly reacts to changes in those values when computing newIndex, calling setCenter(getZoom...), and updating nodes via setNodes.
348-354: SamesetTimeoutinside setState issue.This has the same concern as lines 307-309. Consider tracking
changedexternally and callingsetCenterafter thesetNodescall completes.♻️ Suggested refactor
+ let shouldCenter = false; + let centerTarget = { x: 0, y: 0 }; + setNodes((nds) => { let changed = false; const newNodes = nds.map((n) => { const selected = n.id === targetNode.id; if (n.selected !== selected) changed = true; return { ...n, selected }; }); if (changed) { - const x = targetNode.position.x + NODE_WIDTH / 2; - const y = targetNode.position.y + NODE_HEIGHT / 2; - setTimeout(() => { - setCenter(x, y, { zoom: Math.max(getZoom(), 1), duration: 500 }); - }, 0); + shouldCenter = true; + centerTarget = { + x: targetNode.position.x + NODE_WIDTH / 2, + y: targetNode.position.y + NODE_HEIGHT / 2, + }; } return changed ? newNodes : nds; }); + + if (shouldCenter) { + setCenter(centerTarget.x, centerTarget.y, { zoom: Math.max(getZoom(), 1), duration: 500 }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/GraphView.tsx` around lines 348 - 354, The setCenter call is being deferred with setTimeout inside the same state update flow (see setTimeout, setCenter, getZoom, setNodes and the changed flag), which can cause the same "setState inside setTimeout" issue; instead, detect and store the changed/targetNode info before calling setNodes (or use the setNodes functional update to compute the new node and capture its center), then invoke setCenter after setNodes completes—e.g., track a local variable or state like pendingCenter (x,y) when you setNodes and call setCenter once that update finishes (or from a useEffect that watches pendingCenter) so you remove the setTimeout and reliably center using getZoom and duration.
295-313: Side effect insidesetNodescallback is an anti-pattern.Calling
setTimeoutwithfitViewinside a setState updater function can cause issues:
- In React StrictMode, updater functions may be invoked multiple times, scheduling duplicate
fitViewcalls.- Mixing state logic with side effects reduces predictability.
Consider extracting the side effect to run after the state update using a separate flag or ref.
♻️ Suggested refactor using a ref to track pending fitView
+ const pendingFitViewRef = useRef(false); // In the useEffect: setNodes((nds) => { let hasSelected = false; const newNodes = nds.map((n) => { if (n.selected) hasSelected = true; return { ...n, selected: false }; }); if (hasSelected) { - setTimeout(() => { - fitView({ duration: 800, padding: 0.05 }); - }, 0); + pendingFitViewRef.current = true; return newNodes; } return nds; }); + + // After setNodes, outside the updater: + if (pendingFitViewRef.current) { + pendingFitViewRef.current = false; + fitView({ duration: 800, padding: 0.05 }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/GraphView.tsx` around lines 295 - 313, The updater passed to setNodes currently runs a side effect (setTimeout(()=>fitView(...))) which is an anti-pattern; change this by removing the setTimeout/fitView call from the setNodes callback (the block that computes newNodes and uses hasSelected) and instead set a local flag or a ref (e.g., pendingFitRef) when you detect hasSelected, then use an effect (useEffect) that watches that flag or nodes/matched state to call fitView once after the state update and clear the flag; keep calls to setMatchedNodes, setCurrentMatchIndex, and setErrorMessage as-is but ensure fitView is only invoked from the effect, not inside setNodes or any state updater.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/GraphView.tsx`:
- Line 365: The effect that currently lists [searchString, nodes,
currentMatchIndex] should include the stable functions from
useReactFlow—specifically fitView, setCenter, and getZoom—in its dependency
array to satisfy react-hooks/exhaustive-deps; update the dependency array used
in the effect inside GraphView.tsx (the effect that references fitView,
setCenter, and getZoom) to include these three symbols, and if desired later for
performance split the effect into two (one for filtering on searchString/nodes
and another for centering on currentMatchIndex) to avoid re-running filtering
when navigating matches.
---
Outside diff comments:
In `@src/components/GraphView.tsx`:
- Around line 460-464: The clear button currently calls fitView immediately and
also updates state (setSearchString, setNodes) which can trigger the debounced
effect that may call fitView again; add a skip flag to avoid duplicate fits by
creating a ref (e.g., skipFitOnClearRef) set to true inside the onClick handler
before clearing state, and in the debounced effect that checks
hasSelected/readies fitView, early-return if skipFitOnClearRef.current is true
and then reset it to false; reference setSearchString, setNodes, fitView,
hasSelected and the debounced effect when applying this change.
---
Nitpick comments:
In `@src/components/GraphView.tsx`:
- Around line 66-94: The navigateMatch useCallback currently lists only
matchedNodes in its dependency array but also reads setCenter, getZoom, setNodes
and NODE_WIDTH/NODE_HEIGHT; update the dependency array for the navigateMatch
callback to include setCenter, getZoom, setNodes, NODE_WIDTH, and NODE_HEIGHT
(in addition to matchedNodes) so the hook correctly reacts to changes in those
values when computing newIndex, calling setCenter(getZoom...), and updating
nodes via setNodes.
- Around line 348-354: The setCenter call is being deferred with setTimeout
inside the same state update flow (see setTimeout, setCenter, getZoom, setNodes
and the changed flag), which can cause the same "setState inside setTimeout"
issue; instead, detect and store the changed/targetNode info before calling
setNodes (or use the setNodes functional update to compute the new node and
capture its center), then invoke setCenter after setNodes completes—e.g., track
a local variable or state like pendingCenter (x,y) when you setNodes and call
setCenter once that update finishes (or from a useEffect that watches
pendingCenter) so you remove the setTimeout and reliably center using getZoom
and duration.
- Around line 295-313: The updater passed to setNodes currently runs a side
effect (setTimeout(()=>fitView(...))) which is an anti-pattern; change this by
removing the setTimeout/fitView call from the setNodes callback (the block that
computes newNodes and uses hasSelected) and instead set a local flag or a ref
(e.g., pendingFitRef) when you detect hasSelected, then use an effect
(useEffect) that watches that flag or nodes/matched state to call fitView once
after the state update and clear the flag; keep calls to setMatchedNodes,
setCurrentMatchIndex, and setErrorMessage as-is but ensure fitView is only
invoked from the effect, not inside setNodes or any state updater.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1975abec-cb6f-447f-807b-e256cecf2925
📒 Files selected for processing (2)
.changeset/every-paws-wish.mdsrc/components/GraphView.tsx
|
|
||
| return () => clearTimeout(timeout); | ||
| }, [searchString]); | ||
| }, [searchString, nodes, currentMatchIndex]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check ESLint configuration for react-hooks/exhaustive-deps rule
fd -t f -e js -e json -e yaml -e yml '.eslintrc' --exec cat {} 2>/dev/null | head -50
cat package.json | jq '.eslintConfig // empty'Repository: ioflux-org/studio-json-schema
Length of output: 55
🏁 Script executed:
# First, get the size of GraphView.tsx
wc -l src/components/GraphView.tsxRepository: ioflux-org/studio-json-schema
Length of output: 105
🏁 Script executed:
# Read the relevant section around line 365
sed -n '350,380p' src/components/GraphView.tsxRepository: ioflux-org/studio-json-schema
Length of output: 936
🏁 Script executed:
# Search for useReactFlow usage in the file
rg "useReactFlow" src/components/GraphView.tsx -B 2 -A 2Repository: ioflux-org/studio-json-schema
Length of output: 401
🏁 Script executed:
# Check for ESLint configuration in the repo more thoroughly
find . -type f \( -name ".eslintrc*" -o -name "eslint.config.*" \) 2>/dev/null
cat package.json | jq '.eslintConfig' 2>/dev/null || echo "No eslintConfig in package.json"Repository: ioflux-org/studio-json-schema
Length of output: 96
🏁 Script executed:
# Read a larger section to see the full effect body and where it starts
sed -n '320,375p' src/components/GraphView.tsxRepository: ioflux-org/studio-json-schema
Length of output: 1814
🏁 Script executed:
# Check the ESLint configuration
cat eslint.config.jsRepository: ioflux-org/studio-json-schema
Length of output: 888
🏁 Script executed:
# Check if fitView is used anywhere in the effect starting from line 320 to 375
sed -n '310,380p' src/components/GraphView.tsx | grep -n "fitView"Repository: ioflux-org/studio-json-schema
Length of output: 55
🏁 Script executed:
# Get more context - find where this effect starts to understand its full scope
sed -n '290,375p' src/components/GraphView.tsxRepository: ioflux-org/studio-json-schema
Length of output: 2641
Add missing dependencies to the effect's dependency array.
The effect uses fitView, setCenter, and getZoom but doesn't include them in the dependency array. While these functions from useReactFlow are typically stable, ESLint's react-hooks/exhaustive-deps rule (which is enabled in this project) flags them as missing dependencies.
Additionally, including currentMatchIndex means this effect re-runs when the user navigates via navigateMatch. The guards prevent double-centering, but the effect still performs filtering and comparison work on each navigation. If performance becomes a concern, consider separating the "search on input/nodes change" logic from "center on navigation" logic.
Suggested fix
- }, [searchString, nodes, currentMatchIndex]);
+ }, [searchString, nodes, currentMatchIndex, fitView, setCenter, getZoom]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }, [searchString, nodes, currentMatchIndex]); | |
| }, [searchString, nodes, currentMatchIndex, fitView, setCenter, getZoom]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/GraphView.tsx` at line 365, The effect that currently lists
[searchString, nodes, currentMatchIndex] should include the stable functions
from useReactFlow—specifically fitView, setCenter, and getZoom—in its dependency
array to satisfy react-hooks/exhaustive-deps; update the dependency array used
in the effect inside GraphView.tsx (the effect that references fitView,
setCenter, and getZoom) to include these three symbols, and if desired later for
performance split the effect into two (one for filtering on searchString/nodes
and another for centering on currentMatchIndex) to avoid re-running filtering
when navigating matches.
|
can you please resolve the merge conflicts? |
5ff0658 to
e7a3c1f
Compare
|
hey @AgniveshChaubey ! resolved the merge conflict and rebased onto the latest main. ready for your review whenever you get the chance. |

Summary
Fixes a bug where search results wouldn't update if the graph itself changed while you were searching.
Before, if the graph updated behind a search, the highlights would get stuck. Now, the search automatically refreshes whenever the graph changes, keeping the results accurate.
What kind of change does this PR introduce
Bug fix
Issue Number
Closes #244
Video
Screen.Recording.2026-03-30.at.11.29.25.PM.mov
Does this PR introduce a breaking change?
No
If relevant, did you update the documentation?
Not required
Summary by CodeRabbit